Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace topicMap with topicPartitions in cluster module #439

Merged
merged 3 commits into from Mar 2, 2019

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Aug 7, 2018

Old code use topicMap to map topic to partition count,and partition ID is 0, 1, 2, ... partitionCount - 1.
However, if some partitions are broken (that is to say, these partitions have no leader), each time you call getOffsets() method, fetchMetadata will be set, leading to the result that topic-fresh and offset-fresh these two configurations are meaningless.
To solve this problem, I store each partition ID in a golang slice, when get metadata from the broker, check if partition has a leader, if true, then append it to the slice. I've initialized the slice's capacity with partitionCount, and append can be called at most partitionCount times, so the capacity won't increase.
Other issues:

  1. errorTopics map[string]bool in generateOffsetRequests() is unnecessary, I've found errorTopics is used for some complex logic in older codes.
  2. executeTemplate() in package notifier, the struct has a field named 'ID', but in config/default-http-post.tmpl, it's {{.Id}}.

@coveralls
Copy link

coveralls commented Aug 8, 2018

Coverage Status

Coverage increased (+0.04%) to 74.029% when pulling 932f5f6 on BewareMyPower:master into 12e681a on linkedin:master.

@toddpalino toddpalino merged commit c6a993e into linkedin:master Mar 2, 2019
vixns pushed a commit to vixns/Burrow that referenced this pull request Apr 3, 2019
* consistent with notifier executeTemplate

* replace topicMap with topicPartitions

* add partial partitions update test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants